Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Usage of asserts is incorrect? #1914

Closed
sergekukharev opened this issue Oct 15, 2015 · 19 comments
Closed

Usage of asserts is incorrect? #1914

sergekukharev opened this issue Oct 15, 2015 · 19 comments

Comments

@sergekukharev
Copy link

It's not a secret that all asserts in PHPUnit are in fact static methods, but everyone are using them as they are not, even official documentation.

I think I understand the reasons, why they were made static in first place, probably it is related to performance (btw, do other reasons exists for that?).

But in my own opinion, if they are static, we should use them as we use other static methods to make our code cleaner.

This means using:

self::assertTrue(...);

instead of:

$this->assertTrue(...);

Can anyone give me a good reason why we shouldn't?

@fabiocarneiro
Copy link

👯

@sebastianbergmann
Copy link
Owner

  • This has nothing to do with performance
  • This is due to how JUnit 3.8, which served as the initial inspiration for PHPUnit, did things ~ 15 years ago
  • It allows developers to reuse PHPUnit's assertions and constraints outside the context of PHPUnit
  • A regular user of PHPUnit should use $this-> instead of self::

@VasekPurchart
Copy link
Contributor

VasekPurchart commented Jul 15, 2016

I understand the reasons above (and I find them reasonable), but maybe there can be a slight change?
With growth of static analysis tools for PHP this could provide unnecessary warnings, because in most other cases if you call a static method non-statically that might be an error.

I think that it is certainly useful to allow reuse of asserts and doing it by static calls is perfectly fine in this case, but maybe there could be different behavior directly on TestCase? I mean instead of extending PHPUnit_Framework_Assert why not just call the static method in a new non-static method with the same name?

That means that the API would stay the same - obviously other than the static change, but maybe that is not a big BC break, because everywhere is written to use $this-> with these methods?

@briedis
Copy link

briedis commented Jul 22, 2016

...and I almost went to PhpStorm bug tracker to submit a bug, that $this->assert.. IDE suggestions don't offer anything, but the code still works. I thought I was going insane...

@eclipxe13
Copy link
Contributor

I thinks is great that the methods are defined as static when they are not using $this.

Please also notice:

In other languages like C# its event a hint that if some method don't access instance data (don't use this) can be declared static https://msdn.microsoft.com/en-us/library/ms245046.aspx

Calling a non-method method statically generates on PHP 5 E_STRICT & on PHP 7 E_DEPRECATED. But not the other way around. Call a static method non-statically is fine.

@mcfedr
Copy link

mcfedr commented Aug 11, 2016

What is the reasoning behind the statement?

A regular user of PHPUnit should use $this-> instead of self::

Its what I do, but why is it considered the 'right' choice?

@rtek
Copy link

rtek commented Aug 23, 2016

I think the reason for static assertions is:

  • They do not use object context, so you can reuse them anywhere
  • Calling them via $this-> is valid and safe since it assumes object context (which is not required)
  • If an assertion needs object context in the future, your tests will be safe
  • If you use self:: then your tests will break

@mcfedr
Copy link

mcfedr commented Aug 24, 2016

I can see value in the assertions being declared statically, this probably allows for better code reuse.

But I cannot see the value in calling them via $this - It would be impossible to change the assertions to need object context because it would contradict the reason of code reuse, it would be a very breaking change.

If anything it just hide the fact that they are static and reusability.

@BenMorel
Copy link
Contributor

I still fail to see the motivation behind keeping things as they are.

This has nothing to do with performance

Great then.

This is due to how JUnit 3.8, which served as the initial inspiration for PHPUnit, did things ~ 15 years ago

Fair enough. But still, we're15 years later, 6 major versions of PHPUnit have been released since then, slowly forgetting this heritage doesn't look like a big deal.

It allows developers to reuse PHPUnit's assertions and constraints outside the context of PHPUnit

Now that's a good point.

A regular user of PHPUnit should use $this-> instead of self::

This is where I get lost. You're giving all your reasons to keep the methods static, yet you advise users to call them dynamically, with no explanation whatsoever.

Why on earth should we call them dynamically?

@sebastianbergmann
Copy link
Owner

https://phpunit.de/manual/current/en/appendixes.assertions.html#appendixes.assertions.static-vs-non-static-usage-of-assertion-methods

mhujer added a commit to mhujer/elasticsearch-php that referenced this issue Aug 11, 2017
mhujer added a commit to mhujer/elasticsearch-php that referenced this issue Aug 11, 2017
mhujer added a commit to mhujer/elasticsearch-php that referenced this issue Aug 20, 2017
polyfractal pushed a commit to elastic/elasticsearch-php that referenced this issue Aug 21, 2017
* [TEST] use ::class instead of classname string

Reason - when the class is renamed or removed, it will be detected much faster.
It helps with the static analysis of the code (also used by IDEs)

* [TEST] use assertSame() instead of assertEquals()

assertEquals() means ==
assertSame() is ===

Can cause tests not detecting issues, because different instances of
same class with same data are equal, but not the same.

* [TEST] convert array() to [] syntax

* [TEST] $this->assert* should be used

See
sebastianbergmann/phpunit#1914 (comment)
p365labs pushed a commit to p365labs/elasticsearch-php that referenced this issue Sep 10, 2017
* [TEST] use ::class instead of classname string

Reason - when the class is renamed or removed, it will be detected much faster.
It helps with the static analysis of the code (also used by IDEs)

* [TEST] use assertSame() instead of assertEquals()

assertEquals() means ==
assertSame() is ===

Can cause tests not detecting issues, because different instances of
same class with same data are equal, but not the same.

* [TEST] convert array() to [] syntax

* [TEST] $this->assert* should be used

See
sebastianbergmann/phpunit#1914 (comment)
p365labs pushed a commit to p365labs/elasticsearch-php that referenced this issue Sep 10, 2017
* [TEST] use ::class instead of classname string

Reason - when the class is renamed or removed, it will be detected much faster.
It helps with the static analysis of the code (also used by IDEs)

* [TEST] use assertSame() instead of assertEquals()

assertEquals() means ==
assertSame() is ===

Can cause tests not detecting issues, because different instances of
same class with same data are equal, but not the same.

* [TEST] convert array() to [] syntax

* [TEST] $this->assert* should be used

See
sebastianbergmann/phpunit#1914 (comment)
@dereuromark
Copy link

Fair enough. But still, we're15 years later, 6 major versions of PHPUnit have been released since then, slowly forgetting this heritage doesn't look like a big deal.

I feel with you.
With higher phpstan levels you face the same thing in the default behavior of reporting.
It is a code smell - totally.
And I think it should be fixed up in one direction, both for usage and docs.

@sebastianfeldmann
Copy link
Sponsor

sebastianfeldmann commented Sep 8, 2018

It is a code smell - totally.

No it's not! It's OO. There is no rule, that you can't call static methods within your instance context.
Static methods are callable either way. Only none static methods are not callable statically because they need the object context.
JUnit does the same thing by the way ;)

@dereuromark
Copy link

Static anlyzers, IDEs, everything is warning and complaining about this. Why is this then an issue for those to report if it was totally "acceptable"? Sure, technically it is fine, but conceptually?

@sebastianfeldmann
Copy link
Sponsor

Fix the tools!

@sergekukharev
Copy link
Author

What's the point of fixing tools if we can fix the documentation?

@keradus
Copy link
Contributor

keradus commented Sep 11, 2018

I would advocate to call methods with respect to their declarations as well.

For all who want's to automate choosing his way of usage of assertions, PHP CS Fixer can do it for you with php_unit_test_case_static_method_calls rule - it can be configured to ensure assertions are called in unified way - $this->, self:: or static::

@hopeseekr
Copy link

I have thoroughly analyzed PHPUnit's TestCase and Assert code, and unless we are going to declare willfully disregarding the static method calls are OK, then such an important code quality tool as PHPUnit should have code written for it using self::assertEquals() vs. the contextually ambiguous $this->assertEquals().

Or, PHPUnit needs to have a third class that does something like

// Expose static assert methods for people who need them.
class Assert
{
    public static function assertEquals($expected, $actual, $memo='')
    {
        return (new AssertLogic())->assertEquals($expected, $actual, $memo);
    }
}

Then extend TestCase against AssertLogic. Then literally everyone can be happy, at probably minimal overhead. (if the news are too much, then private static $asserter.

@VasekPurchart
Copy link
Contributor

For anyone that is interested we have since moved to using Assert::assert*() methods directly. Since these are basically functions (no state) and do not need to be connected to TestCase, it makes more sense.

The only downside is that you have to add use for Assert (and potentially other classes), but I don't mind that at all. Plus it becomes clearer if you use some custom asserts where they come from and you don't have to "register" them in TestCase either.

@BenMorel
Copy link
Contributor

I like this idea 👍

But to better comply with PHPUnit standards, I won't be calling Assert::assert*() statically, I'll prefer:

(new class extends Assert {})->assert*();

...

</joke_mode>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests